-
-
Notifications
You must be signed in to change notification settings - Fork 113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(stdlib): Correctly promote numbers to bigints when left-shifting #1354
Conversation
stdlib/runtime/numbers.gr
Outdated
@@ -1833,7 +1833,13 @@ export let (<<) = (x: Number, y: Number) => { | |||
} else { | |||
let xval = coerceNumberToWasmI64(x) | |||
let yval = coerceNumberToWasmI64(y) | |||
WasmI32.toGrain(reducedInteger(WasmI64.shl(xval, yval))): Number | |||
if (WasmI64.leU(WasmI64.clz(xval), yval)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this fast path only works for positive numbers—is there any other quick check that'd also work for negative numbers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, this actually has the opposite effect that you are imagining: clz
of a negative number is zero, so we always promote to a big integer. If you think that's a problem, I can try to refine the condition a bit more, but I just tested this program, and it works as intended:
import BigInt from "bigint"
print(5 << 64)
print(-5 << 64)
print(BigInt.shl(-5t, 64l))
output:
92233720368547758080
-92233720368547758080
-92233720368547758080
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if I wasn't clear—I wasn't trying to say that this wasn't correct, just that exactly your point—clz
of a negative number is always zero, so negative numbers never make it to the fast path. I think it'd be better to have a check that entered the fast path for both positive and negative numbers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ospencer Got it. I think that the version I just pushed should do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't be too annoyed at me, but I like the version you originally did better than this one. &&
and ||
are implemented with branching, so now it's pretty expensive to do this check, almost to the point that it might be better to just skip the check altogether and always promote. I think I'm fine to have just the positive case have a chance to be fast-tracked.
Another option would be doing an abs of the value and then doing the clz—can do an abs in like 3 instructions so it's not too much more than your original version (but up to you).
@ospencer I did the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks for bearing with me on the iterations 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clever code!
Closes #1350.